Skip to content

Conversation

@Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Oct 9, 2024

Relates #455

For personal testing a created a branch targeting PyO3 0.23-dev from main. I thought I'll leave that here. Once pyo3 0.23 releases, this can probably be split up to migrate.

@Icxolu
Copy link
Contributor Author

Icxolu commented Nov 18, 2024

@davidhewitt I just bumped this to the 0.23 release version and run into the Sync restriction for #[pyclass]es on PySliceContainer. I believe it should be fine to implement Sync if the elements are also Sync (patch below) but I'm not 100% certain. Your opinion would be appreciated.

Sync Patch
diff --git a/src/dtype.rs b/src/dtype.rs
index a97edd43..ac96a277 100644
--- a/src/dtype.rs
+++ b/src/dtype.rs
@@ -492,7 +492,7 @@ impl Sealed for Bound<'_, PyArrayDescr> {}
 ///
 /// [enumerated-types]: https://numpy.org/doc/stable/reference/c-api/dtype.html#enumerated-types
 /// [data-models]: https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
-pub unsafe trait Element: Sized + Send {
+pub unsafe trait Element: Sized + Send + Sync {
     /// Flag that indicates whether this type is trivially copyable.
     ///
     /// It should be set to true for all trivially copyable types (like scalar types
diff --git a/src/slice_container.rs b/src/slice_container.rs
index b1326746..2ba31181 100644
--- a/src/slice_container.rs
diff --git a/src/dtype.rs b/src/dtype.rs
index a97edd43..ac96a277 100644
--- a/src/dtype.rs
+++ b/src/dtype.rs
@@ -492,7 +492,7 @@ impl Sealed for Bound<'_, PyArrayDescr> {}
 ///
 /// [enumerated-types]: https://numpy.org/doc/stable/reference/c-api/dtype.html#enumerated-types
 /// [data-models]: https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
-pub unsafe trait Element: Sized + Send {
+pub unsafe trait Element: Sized + Send + Sync {
     /// Flag that indicates whether this type is trivially copyable.
     ///
     /// It should be set to true for all trivially copyable types (like scalar types
diff --git a/src/slice_container.rs b/src/slice_container.rs
index b1326746..2ba31181 100644
--- a/src/slice_container.rs
+++ b/src/slice_container.rs
@@ -14,8 +14,9 @@ pub(crate) struct PySliceContainer {
 }
 
 unsafe impl Send for PySliceContainer {}
+unsafe impl Sync for PySliceContainer {}
 
-impl<T: Send> From<Box<[T]>> for PySliceContainer {
+impl<T: Send + Sync> From<Box<[T]>> for PySliceContainer {
     fn from(data: Box<[T]>) -> Self {
         unsafe fn drop_boxed_slice<T>(ptr: *mut u8, len: usize, _cap: usize) {
             let _ = Box::from_raw(ptr::slice_from_raw_parts_mut(ptr as *mut T, len));
@@ -39,7 +40,7 @@ impl<T: Send> From<Box<[T]>> for PySliceContainer {
     }
 }
 
-impl<T: Send> From<Vec<T>> for PySliceContainer {
+impl<T: Send + Sync> From<Vec<T>> for PySliceContainer {
     fn from(data: Vec<T>) -> Self {
         unsafe fn drop_vec<T>(ptr: *mut u8, len: usize, cap: usize) {
             let _ = Vec::from_raw_parts(ptr as *mut T, len, cap);
@@ -65,7 +66,7 @@ impl<T: Send> From<Vec<T>> for PySliceContainer {
 
 impl<A, D> From<ArrayBase<OwnedRepr<A>, D>> for PySliceContainer
 where
-    A: Send,
+    A: Send + Sync,
     D: Dimension,
 {
     fn from(data: ArrayBase<OwnedRepr<A>, D>) -> Self {

@juntyr
Copy link

juntyr commented Nov 18, 2024

@Icxolu Thank you for working on this PR - I just started looking into the PyO3 version bumps in my project and it's always awesome to see ongoing work <3

@davidhewitt
Copy link
Member

Thanks for pushing forward on this! It might be easier to review the change to unsafe impl Sync for PySliceContainer as a separate PR? That seems like it justifies more scrutiny than the large but mostly mechanical upgrades going on here.

For the Sync patch above, it seems to me that depends on what numpy's access rules are. I guess you do get a pointer into the underlying array memory to read elements in-place, so it seems correct that Element: Sync should be required.

@Icxolu
Copy link
Contributor Author

Icxolu commented Nov 19, 2024

It might be easier to review the change to unsafe impl Sync for PySliceContainer as a separate PR? That seems like it justifies more scrutiny than the large but mostly mechanical upgrades going on here.

Good idea, opened #469 for that.

@Icxolu Icxolu force-pushed the pyo3-0.23 branch 2 times, most recently from da18a72 to d35c53a Compare November 20, 2024 19:02
@Icxolu
Copy link
Contributor Author

Icxolu commented Nov 20, 2024

PyO3 dropped support for PyPy 3.7 and 3.8 in 0.23. I addressed this in #470

@Icxolu Icxolu marked this pull request as ready for review November 20, 2024 20:40
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me! I'll follow up with an addition to CI for free-threaded testing, and if that seems ok, let's ship!

@davidhewitt davidhewitt merged commit c386c8e into PyO3:main Nov 22, 2024
67 of 68 checks passed
@Icxolu Icxolu deleted the pyo3-0.23 branch November 22, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants